-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic asyncio support #43944
base: main
Are you sure you want to change the base?
Add basic asyncio support #43944
Conversation
Is this related to AIP-70? |
I think you prob know the answer. The AIP deals with asyncio, so yes strictly speaking related in that sense. But not "part of". That AIP has been in draft a long time not sure actively worked on. Anyway, I don't think what I'm doing here really requires an AIP. |
There is a similar draft PR open as POC for it though not active. So just wanted to confirm. Thanks for the details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really exciting.
Do we want an @provide_async_session
as a follow up ? (I think Ash wanted to kill that decorator at some point, even if I find it kind of useful actually)
@pierrejeambrun A FastAPI dep to give an async session would be good, but I think anything in the models etc should be passed an explicit session. |
NICE! Yeah. I really like to see it working with mysql compatibility and benchmarks 📦 (because of course MySQL is our beloved database) |
i would say let's wait and see how / where / when we need it |
0af9d4d
to
620902e
Compare
Lovely! Thank you for adding this, will try to start using this soon :) |
5765c6c
to
8b5ab77
Compare
This is meant to be sort of a hello world for asyncio support in airflow. It will be refined and extended in the future. But I think airflow ultimately really needs to go in this direction: in the new REST API, in the new AIP-72 internal API server, in triggers, and ultimately, in the scheduler.
8b5ab77
to
6791e41
Compare
@dstandish in lowest direct dep tests, we may need to limit minimum version for More info on this slack thread. |
session = create_async_session() | ||
session.add(Log(event="hihi1234")) | ||
await session.commit() | ||
l = await session.scalar(select(Log).where(Log.event == "hihi1234").limit(1)) # noqa: E741 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can avoid this noqa by just change the var name :)
l = await session.scalar(select(Log).where(Log.event == "hihi1234").limit(1)) # noqa: E741 | ||
assert l.event == "hihi1234" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l = await session.scalar(select(Log).where(Log.event == "hihi1234").limit(1)) # noqa: E741 | |
assert l.event == "hihi1234" | |
log_result = await session.scalar(select(Log).where(Log.event == "hihi1234").limit(1)) # noqa: E741 | |
assert log_result.event == "hihi1234" |
This is meant to be sort of a hello world for asyncio support in airflow. It will be refined and extended in the future. E.g. probably we would add more config flexibility re the libraries, connect args etc. But it's good to start simple.
Anyway, ultimately, I think Airflow really needs to go in this direction: in the new REST API, in the new AIP-72 internal API server, in triggers, and ultimately, in the scheduler.